Closed
Bug 557552
Opened 14 years ago
Closed 14 years ago
add ability to view performance test results between two pushes
Categories
(Tree Management Graveyard :: TBPL, defect)
Tree Management Graveyard
TBPL
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dietrich, Assigned: dietrich)
References
Details
Attachments
(3 files, 4 obsolete files)
finding the performance difference between two revisions is cumbersome business with TBPL. lots of clicking around, and copy/pasting testnames and result numbers just to compare a single test's results between two pushes. this patch provides simple UI for viewing at-a-glance all results for all completed performance tests between two pushes. usage: - check two boxes to trigger the comparison - click the resulting table to close it i have a few improvements planned to make it easier to use (noted in the patch) but it's far enough along to be useful as it is.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
Updated•14 years ago
|
Attachment #437331 -
Attachment is patch: true
Attachment #437331 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 3•14 years ago
|
||
moved the URL abstraction out to bug 557641.
Attachment #437331 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dietrich
Comment 4•14 years ago
|
||
Comment on attachment 437403 [details] [diff] [review] v2 First of all, thank you very much for doing this! > '<h2><span class="pusher">' + push.pusher + '</span> – ' + >- '<span class="date">' + self._getDisplayDate(push.date) + '</span></h2>\n' + >+ '<span class="date">' + self._getDisplayDate(push.date) + '</span>' + >+ ' (compare: <input class="revsToCompare" id="compareRevs" type="checkbox" value="' + push.patches[0].rev + '" onclick="compare()">)' + >+ '</h2>\n' + Please set up the click handler like the other ones (see for example _installMachineResultClickHandler). >diff --git a/script.js b/script.js Don't put the compare function into script.js. Move this stuff into UserInterface.js or create a new file, for example PerformanceComparator.js or something like that. >+ for (var i in revsEls) Use Array.forEach(revsEls, function ...). Only use for...in for objects. See the note on https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Statements/For...in#Description for reference. >+ if (revsEls[i].checked) revs.push(revsEls[i].getAttribute("value")); Give the if body its own line and use .value instead of .getAttribute("value"). >+ var oss = Controller._data.getOss(); >+ var revResults = []; >+ var pushes = Controller._data.getPushes(); >+ pushes.forEach(function(push) { >+ var rev = push.patches[0].rev; >+ if (revs.some(function(r) r == rev)) { Data.js has _getPushForRev. >+ var revResult = { rev: rev }; >+ oss.forEach(function(os) { >+ revResult[os] = {}; >+ if (push.results && push.results[os]) { >+ for (var buildType in push.results[os]) { >+ if (buildType != "Talos") continue; >+ var buildResults = push.results[os][buildType]; >+ buildResults.forEach(function(buildResult) { >+ var testResults = Controller._data.getMachineResults()[buildResult.runID].getTestResults(); >+ if (testResults) { >+ testResults.forEach(function(testResult) { >+ revResult[os][testResult.name] = testResult.result; >+ }); >+ } >+ }); >+ } >+ } >+ }); >+ revResults.push(revResult); >+ } >+ }); I think this should all be done somewhere in Data.js, probably after _combineResults, once for all pushes. You can set a perfResults property on every push object. >+ var osNames = {"linux": "Linux", "linux64": "Linux64", "osx": "Mac OS X", "osx64": "Mac OS X64", "windows": "Windows"}; This needs to be factored out into Config.js. >+ var testNames = [ >+ "tdhtml", ... >+ "twinopen" >+ ]; This list needs to live somewhere else, maybe in TinderboxData.js. >+ div.setAttribute("style", "position: absolute; z-index: 1000; left: 0px; top: 0px; background-color: #ffffff; border: 1px solid black; font-size: 0.6em;"); Please put this into style.css and set an ID on the div. >+ var revsEls = document.getElementsByClassName("revsToCompare"); >+ for (var i in revsEls) revsEls[i].checked = false; I think with the help of jQuery this could become: $(".revsToCompare").each(function { this.checked = false; }) ... or maybe even shorter.
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4) > >+ for (var i in revsEls) > > Use Array.forEach(revsEls, function ...). Only use for...in for objects. See > the note on > https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Statements/For...in#Description > for reference. It's a NodeList, not an array. Other changes seem fine, new patch coming.
Assignee | ||
Comment 6•14 years ago
|
||
Comments addressed. All UI stuff is now separated out into UserInterface.js.
Attachment #437403 -
Attachment is obsolete: true
Comment 7•14 years ago
|
||
(In reply to comment #5) > (In reply to comment #4) > > >+ for (var i in revsEls) > > > > Use Array.forEach(revsEls, function ...). Only use for...in for objects. See > > the note on > > https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Statements/For...in#Description > > for reference. > > It's a NodeList, not an array. That means you can't do revsEls.forEach(...), but you can still do Array.forEach(revsEls, ...) if I recall correctly.
Assignee | ||
Comment 8•14 years ago
|
||
Yep, works on NodeLists.
Attachment #439176 -
Attachment is obsolete: true
Attachment #439183 -
Flags: review?(mstange)
Comment 9•14 years ago
|
||
Comment on attachment 439183 [details] [diff] [review] v3.1 Thanks!
Attachment #439183 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/1d8387bacfbb
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 11•14 years ago
|
||
Could you also check in PerformanceComparator.js, please? :-) I should have noticed that in the review... Backed out for now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•14 years ago
|
||
with file attached
Attachment #439183 -
Attachment is obsolete: true
Attachment #439332 -
Flags: review+
Assignee | ||
Comment 13•14 years ago
|
||
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/d214660065d3
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 14•14 years ago
|
||
Does tbpl implement Array.forEach if it's not already implemented? It's a non-standard extension (not entirely sure why it wasn't in ES5 to complement the prototype-based counterparts, to be honest), so you need to add it to keep tbpl working in webkit-based browsers, like so somewhere: if (!Array.forEach) { Array.forEach = function (arraylike, fun, thisp) { return Array.prototype.forEach.call(arraylike, fun, thisp); }; }
Comment 15•14 years ago
|
||
It doesn't, so maybe we should just use some jQuery magic.
Updated•10 years ago
|
Product: Webtools → Tree Management
Updated•9 years ago
|
Product: Tree Management → Tree Management Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•